feat: switch from num-bigint-dig to crypto-bigint #394
Conversation
|
@tarcieri updated to the latest version, and fixed a subtle padding bug that I found, please take a final look before I merge |
dignifiedquire
force-pushed
the
const-crypto-biguint
branch
from
7e9dffd to
3a0201c
Compare
|
|
||
| /// Generate a new signing key with a prefix for the digest `D`. | ||
| pub fn random<R: CryptoRngCore + ?Sized>(rng: &mut R, bit_size: usize) -> Result<Self> { | ||
| pub fn random<R: CryptoRngCore>(rng: &mut R, bit_size: usize) -> Result<Self> { |
There was a problem hiding this comment.
Is there a particular reason you're getting rid of ?Sized on these? It makes it possible to use a dyn CryptoRngCore
There was a problem hiding this comment.
Is there a particular reason you're getting rid of ?Sized on these? It makes it possible to use a dyn CryptoRngCore
I don't think so, can revert probably
There was a problem hiding this comment.
I checked, and I removed it because I can't pass that constraint to crypto_primes
There was a problem hiding this comment.
Adding it to crypto-primes is unfortunately blocked by crypto-bigint not supporting it.
There was a problem hiding this comment.
Made RustCrypto/crypto-bigint#760 for that
src/pss.rs
Outdated
|
|
||
| let em = BoxedUint::from_be_slice( | ||
| &em, | ||
| crate::traits::keys::PublicKeyParts::n_bits_precision(priv_key), |
There was a problem hiding this comment.
Similar concern re: PublicKeyParts
| #[cfg(feature = "std")] | ||
| let inner = inner | ||
| .map_err(|e| Box::new(e) as Box<dyn core::error::Error + Send + Sync + 'static>)?; | ||
| #[cfg(not(feature = "std"))] | ||
| let inner = inner.map_err(|_| signature::Error::new())?; |
There was a problem hiding this comment.
Ugh, should really fix this upstream in the signature crate
|
@dignifiedquire can we land this? |
|
There's a minor thing of needing the |
|
Also the 6-10x performance drop is quite surprising, but that can be addressed in a follow-up as well. |
I removed that, there is some other follow ups from you and @tarcieri I wanted to get to, but we can also merge as is |
|
Thank you everyone who helped make this a reality! And sorry for this taking so long.. |
Notably includes #394 which migrates to `crypto-bigint`
Very, very WIPNot anymore, this is ready for review.
Replaces all usage of
num-bigint-digbasedBigIntusage with the newcrypto-bigintcrate, usingBoxedUintCurrent known issue is that we do have a performance regression, which will be able to get rid of over time:
TODOs
RsaPrivateKeyRsaPublicKeydecryptimplementationBigUintto return owned versions